Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PAYARA-1896 Unified asadmin command to set health check notifier configuration #3663

Merged
merged 5 commits into from Feb 8, 2019

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Jan 29, 2019

This PR is only one part of the effort of PAYARA-1896 split for easier review.

Goal was to combine the various asadmin commands to set individual health check notifier configurations into one command. To not break existing commands these were deprecated and a new command set-healthcheck-service-notifier-configuration was added.

The working principle of the new service is largely based on the BaseHealthCheckNotifierConfigurer that was the base class for the various specific commands.

The new command has an additional parameter to specify the type of notifier the setting should apply to. The list of possible values is based on the enum NotifierType. The code is entirely written so newly added types of notifiers do not require changes to this service/command.

Like the BaseHealthCheckNotifierConfigurer this implementation uses a default value for noisy resolved from the corresponding NotifierConfiguration. Unlike the
BaseHealthCheckNotifierConfigurer implementation this value is not resolved by running another command and resolving the flag from the ActionReport. Instead the corresponding NotifierConfiguration instance is directly resolved from the NotificationServiceConfiguration so that the flag can be read directly using NotifierConfiguration#getNoisy(). This allows services to be less entangled and reliant upon each other which should result in a more stable behaviour.

The new command also prints feedback on the changes to the config. It will only apply and report actual changes, that is a setting changing from true to false or vice versa.

Example session:

Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> set-healthcheck-service-notifier-configuration --enabled=false --notifier=jms --noisy=true
jms.noisy was false set to true

Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> set-healthcheck-service-notifier-configuration --enabled=true --notifier=jms --noisy=false
jms.enabled was false set to true
jms.noisy was true set to false

Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> set-healthcheck-service-notifier-configuration --enabled=true --notifier=jms --noisy=false
Command set-healthcheck-service-notifier-configuration executed successfully.
asadmin> 

@jbee jbee self-assigned this Jan 29, 2019
@mulderbaba mulderbaba added this to the 5.191 milestone Jan 29, 2019
@mulderbaba
Copy link
Contributor

jenkins test

@smillidge smillidge changed the title Unified asadmin command to set health check notifier configuration Unified asadmin command to set health check notifier configuration PAYARA-1896 Jan 29, 2019
@smillidge
Copy link
Contributor

Can you remember to use the JIRA number in commit comments and PR names. That way JIRA picks up all the changes and associates with your PR.

report.setExtraProperties(extraProperties);
}
try {
notifierType = NotifierType.valueOf(notifierName.toUpperCase());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally chose to not use acceptableValues to allow the command adopt automatically to changes to the enum.

@jbee
Copy link
Contributor Author

jbee commented Jan 30, 2019

jenkins test please

@mulderbaba mulderbaba changed the title Unified asadmin command to set health check notifier configuration PAYARA-1896 PAYARA-1896 Unified asadmin command to set health check notifier configuration Jan 31, 2019
Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Styling

* {@link NotificationServiceConfiguration} to use its {@link NotifierConfiguration#getNoisy()} as automatic fallback
* setting.
*
* @author jan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full name please

final Notifier notifier = selectByType(Notifier.class, config.getNotifierList());
try {
if (notifier == null) {
ConfigSupport.apply((SingleConfigCode<HealthCheckServiceConfiguration>) proxy -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configProxy or healthCheckServiceConfigProxy please

}

private <T> T selectByType(Class<? super T> commonInterface, List<T> candidates) {
for (T e : candidates) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use single character variables please

@jbee
Copy link
Contributor Author

jbee commented Feb 5, 2019

jenkins test please

@Pandrex247 Pandrex247 merged commit a295df0 into payara:master Feb 8, 2019
jbee pushed a commit to jbee/Payara that referenced this pull request Feb 14, 2019
…ommands

PAYARA-1896 Unified asadmin command to set health check notifier configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants